-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow customization of autotimestamp dateCreated and lastUpdated properties #1855
base: 9.0.x
Are you sure you want to change the base?
Allow customization of autotimestamp dateCreated and lastUpdated properties #1855
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomAutotimeStampSpec
fails when running the tck against gorm-hibernate5
:
Condition failed with Exception:
r.modified != null && r.modified < new Date()
| |
| java.lang.NullPointerException: Cannot get property 'modified' on null object
| at grails.gorm.tests.CustomAutotimeStampSpec.Test when the auto timestamp properties are customized, they are correctly set(CustomAutotimeStampSpec.groovy:15)
null
@matrei This is because hibernate5 needs to be updated after this is released. This will need to be released first in order to do that. It should not break anything because the changes are backwards compatible. |
I'm not sure about the mapping syntax. It does not line up with what I would expect. Perhaps even better would be to use the But you still need to explicitly create those matching properties in the class as well, which is not super intuitive. So now I start to think about annotating the properties instead with |
@matrei if you did annotations, what would happen if you have multiple |
If it is disabled globally you would have to enable it explicitly in the class, right?
That is up to us to decide. |
@matrei how do you disable it globally? Are you suggesting to changing the default behavior to be disabled by default? https://docs.grails.org/6.1.0/ref/Database%20Mapping/autoTimestamp.html |
https://gorm.grails.org/latest/hibernate/manual/index.html#ormdsl
No |
@matrei
class User {
String username
String password
@AutoTimestamp(false)
Date created
@AutoTimestamp
Date modified
} thoughts? |
I'm a fan of this updated approach. I'm ok with the annotation or the mapping syntax. I like the annotation - it seems like we should have a wider discussion if we should move configuration to Annotations so we get better IDE support. |
@matrei annotation is done. There is no restriction on the number of timestamp properties you can have on an entity. Adding multiple annotations or haveing a combination of properties such as Date dateCreated
Date lastUpdate
@AutoTimestamp(false) LocalDateTime created
@AutoTimestamp LocalDateTime modified is now supported. Could be useful in cases where you want to migrate away from using the previous property. Although it is a 1 time scan, I am leaning toward turning this into an ASTTransformation, but don't want to go that deep until everyone gives this the thumbs up. |
Cool! I think |
well, I am a leaning a little against the extra attribute. The IDE displays what the value attribute does. I could prefix the doc with onUpdate. Think of it more like calling a method @AutoTimestamp(boolean onUpdate)
@AutoTimestamp(false)
@AutoTimestamp(true) |
switched to enum class User {
String username
String password
@AutoTimestamp(ON_INSERT) Date created
@AutoTimestamp(ON_UPDATE) Date updated
@AutoTimestamp Date modified // default
} or @AutoTimestamp(INSERT) Date created
@AutoTimestamp(UPDATE) Date updated or @AutoTimestamp(INSERTION) Date created
@AutoTimestamp(UPDATE) Date updated or Grails controller terminology @AutoTimestamp(SAVE) Date created
@AutoTimestamp(UPDATE) Date updated @matrei ? @jdaugherty which one do you like? |
The enum is better. We should use the same terms similar to the method names here: https://gorm.grails.org/latest/hibernate/manual/index.html#eventsAutoTimestamping So UPDATE & INSERT |
It doesn't necessarily correlate because UPDATE is called for INSERT as well so using the word INSERT may add confusion due to ambiguity. Another possibility is to keep it in line with how it previosuly worked and use: @AutoTimestamp(CREATED) Date created
@AutoTimestamp(UPDATED) Date updated or @AutoTimestamp(CREATE) Date created
@AutoTimestamp(UPDATE) Date updated which is in line with Date dateCreated
Date lastUpdated This functionality continues to work. |
I am starting to lean toward: @AutoTimestamp(CREATED) Date created
@AutoTimestamp(UPDATED) Date updated to keep it in sync with how it worked historically. |
Is Update always called? I thought that depends on what the developer does. But yeah, it would make this easier to adopt if we kept the same terminology, so I'd be fine with CREATED / UPDATED. |
yes, currently (pre PR) if you have Date dateCreated
Date lastUpdated
The only way this does not happen is if you have static mapping {
autoTimestamp false
} |
This strikes a good balance between readability and conciseness. @AutoTimestamp(CREATED) Date created
@AutoTimestamp Date updated
@DateCreated Date created2
@LastUpdated Date updated2 |
IMO, I just don’t think ‘ @DateCreated ’ clearly indicates what it is doing as opposed to AutoTimestamp had clear implications. The truth is the only reason I started this in the first place is I hate dateCrested. I don’t think a field name should specify type and it is totally inconsistent. Why isn’t lastUpdated dateLastUpdated or dateUpdated for consistency?
… not a fan 🙄 and definitely don’t want to persist that to a database |
I would prefer the AutoTimestamp that Scott proposed because if you search the code base you can find the code thats doing it. Mocking this stuff in test is always annoying because I can't remember the class that does it and this annotation name would help with that. |
I believe with this change it's still backwards compatible so dateCreated and lastUpdated can still be used. We can document the Annotation for the customization option. |
@jdaugherty I'm with you on that. The suggestion was to create "aliases", so both ways would work. I'm not advocating strongly for it, I'm just brainstorming. As this is an addition to the Grails public API, we could flag it for discussion and take it up in the meeting to gather broader input. |
I’m not against aliasing, but if we did the alias, I would want to go back to having a Boolean argument for AutoTimestamp
I do think the alias is are overkill though because if people really like @DateCreated, they should just use Date dateCreated and not use an annotation at all. The annotation is for people who don’t like dateCreated. |
All these things can be changed before the next milestone. I recommend we move forward with this PR as I need to build off of it and it is delaying my progress. There is no commitment on any naming until we release an RC version. We can create a separate ticket to refine this immediately after it is merged |
To be honest, and I hope to no offense, I think this types of ad hoc changes, possibly belong more in a 7.1 version. We should be laser focused on getting a stable, backwards-compatible, 7.0 version out the door, preferably yesterday, and this is diverting attention and resources from the core goal. |
Isn't the whole point of milestones to introduce features? Aren't milestones subject to major changes? Don't new features best belong in major releases? We aren't even close to the RC phase yet. That's not a reason to stagnate ingenuity. What attention and resources is this diverting? I am the only one that has worked on this. You suggested making it an annotation. I took the extra time to make that happen. |
No, that would not be my definition.
Only if they are breaking backwards compatibility.
We should be.
In my opinion, adding to the public API of a framework is not something that should be rushed and should be scrutinized by as many people as possible to get it right. By wanting to commit new features, in this development stage, others are forced to spend time thinking about this feature instead of more pressing things. Also it's adding to the pile of things to test and document. |
ok, so you are saying that we are close to RC1 when we haven't finished releasing M1? How quickly should a project go from M1 to RC1? Grails 6 M1 was Dec 8 2022. Grails 6 RC1 was May 2023 (6 months later). This was when Grails 6 was fully funded with a paid development team and the transition from 5 to 6 was minimal. The only difference between 5 and 6 was an update from Java 8 to Java 11 and the removal of major features that were added back in 7. To put in perspective, Grace framework is based off of Grails 5 and didn't use a single commit from Grails 6. The changes from 6 to 7 are extreme and exponentially greater than the changes from 5 to 6. How soon after M1 are you targeting RC1? |
No, I’m not suggesting we are close; I’m emphasizing that we should be closer at this point in time.
As soon as we think Grails 7 is release-ready. |
So I shouldn't introduce a better way of doing things that is backwards compatible because we should be in a point in time that we are not? An M1 release or M2 release is a perfect time to refine something. We should not be rushing to put out something that is not thoroughly tested by the public. That takes time. That is point of release candidates. Milestone releases are meant for progress. At least, this is the release strategy that Spring uses or am I mistaken? Does Spring Framework introduce new features between milestone releases? The reality is we are just getting to M1. A lot of hard work has gone into getting us to this point. There is still more work to be done. Let's not discredit that and put out a mediocre product. |
Yes. |
Allows: